From a9fd1c2c05b8fca16d44ac2f930ff4f7d6188f1e Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Fri, 1 Apr 2016 18:06:20 -0700 Subject: [PATCH] Change `Config::target_dir` to return Filesystem This is a shared directory among multiple Cargo processes so access to it needs to be properly synchronized. This commit changes the API of `Config::target_dir` and then propagates the changes outward as necessary. One fallout of this change is now we allow release/debug builds to proceed in parallel. --- src/cargo/ops/cargo_clean.rs | 15 ++++++-- src/cargo/ops/cargo_doc.rs | 4 ++ src/cargo/ops/cargo_install.rs | 7 +++- src/cargo/ops/cargo_package.rs | 58 +++++++++++++++-------------- src/cargo/ops/cargo_rustc/layout.rs | 24 ++++++++---- src/cargo/ops/cargo_rustc/mod.rs | 22 +++++------ src/cargo/ops/registry.rs | 6 +-- src/cargo/util/config.rs | 14 +++---- src/cargo/util/flock.rs | 15 +++++++- src/crates-io/lib.rs | 8 ++-- tests/test_cargo_concurrent.rs | 34 ++++++++++++++++- 11 files changed, 135 insertions(+), 72 deletions(-) diff --git a/src/cargo/ops/cargo_clean.rs b/src/cargo/ops/cargo_clean.rs index c4918f9c3..a8ba8f28a 100644 --- a/src/cargo/ops/cargo_clean.rs +++ b/src/cargo/ops/cargo_clean.rs @@ -21,17 +21,24 @@ pub fn clean(manifest_path: &Path, opts: &CleanOptions) -> CargoResult<()> { // If we have a spec, then we need to delete some packages, otherwise, just // remove the whole target directory and be done with it! + // + // Note that we don't bother grabbing a lock here as we're just going to + // blow it all away anyway. if opts.spec.is_empty() { + let target_dir = target_dir.into_path_unlocked(); return rm_rf(&target_dir); } let (resolve, packages) = try!(ops::fetch(manifest_path, opts.config)); let dest = if opts.release {"release"} else {"debug"}; - let host_layout = Layout::new(opts.config, &root, None, dest); - let target_layout = opts.target.map(|target| { - Layout::new(opts.config, &root, Some(target), dest) - }); + let host_layout = try!(Layout::new(opts.config, &root, None, dest)); + let target_layout = match opts.target { + Some(target) => { + Some(try!(Layout::new(opts.config, &root, Some(target), dest))) + } + None => None, + }; let cx = try!(Context::new(&resolve, &packages, opts.config, host_layout, target_layout, diff --git a/src/cargo/ops/cargo_doc.rs b/src/cargo/ops/cargo_doc.rs index a818ec80e..5b271567e 100644 --- a/src/cargo/ops/cargo_doc.rs +++ b/src/cargo/ops/cargo_doc.rs @@ -50,8 +50,12 @@ pub fn doc(manifest_path: &Path, } }; + // Don't bother locking here as if this is getting deleted there's + // nothing we can do about it and otherwise if it's getting overwritten + // then that's also ok! let target_dir = options.compile_opts.config.target_dir(&package); let path = target_dir.join("doc").join(&name).join("index.html"); + let path = path.into_path_unlocked(); if fs::metadata(&path).is_ok() { let mut shell = options.compile_opts.config.shell(); match open_docs(&path) { diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs index dfb018e6f..916e35bd7 100644 --- a/src/cargo/ops/cargo_install.rs +++ b/src/cargo/ops/cargo_install.rs @@ -82,9 +82,9 @@ pub fn install(root: Option<&str>, let target_dir = if source_id.is_path() { config.target_dir(&pkg) } else { - config.cwd().join("target-install") + Filesystem::new(config.cwd().join("target-install")) }; - config.set_target_dir(&target_dir); + config.set_target_dir(target_dir.clone()); let compile = try!(ops::compile_pkg(&pkg, Some(source), opts).chain_error(|| { human(format!("failed to compile `{}`, intermediate artifacts can be \ found at `{}`", pkg, target_dir.display())) @@ -108,6 +108,9 @@ pub fn install(root: Option<&str>, } if !source_id.is_path() { + // Don't bother grabbing a lock as we're going to blow it all away + // anyway. + let target_dir = target_dir.into_path_unlocked(); try!(fs::remove_dir_all(&target_dir)); } diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs index 277b88eae..a4758e737 100644 --- a/src/cargo/ops/cargo_package.rs +++ b/src/cargo/ops/cargo_package.rs @@ -1,6 +1,7 @@ -use std::io::prelude::*; use std::fs::{self, File}; -use std::path::{self, Path, PathBuf}; +use std::io::SeekFrom; +use std::io::prelude::*; +use std::path::{self, Path}; use tar::{Archive, Builder, Header}; use flate2::{GzBuilder, Compression}; @@ -8,14 +9,14 @@ use flate2::read::GzDecoder; use core::{SourceId, Package, PackageId}; use sources::PathSource; -use util::{self, CargoResult, human, internal, ChainError, Config}; +use util::{self, CargoResult, human, internal, ChainError, Config, FileLock}; use ops; pub fn package(manifest_path: &Path, config: &Config, verify: bool, list: bool, - metadata: bool) -> CargoResult> { + metadata: bool) -> CargoResult> { let path = manifest_path.parent().unwrap(); let id = try!(SourceId::for_path(path)); let mut src = PathSource::new(path, &id, config); @@ -39,29 +40,37 @@ pub fn package(manifest_path: &Path, let filename = format!("{}-{}.crate", pkg.name(), pkg.version()); let dir = config.target_dir(&pkg).join("package"); - let dst = dir.join(&filename); - if fs::metadata(&dst).is_ok() { - return Ok(Some(dst)) - } + let mut dst = match dir.open_ro(&filename, config, "packaged crate") { + Ok(f) => return Ok(Some(f)), + Err(..) => { + let tmp = format!(".{}", filename); + try!(dir.open_rw(&tmp, config, "package scratch space")) + } + }; // Package up and test a temporary tarball and only move it to the final // location if it actually passes all our tests. Any previously existing // tarball can be assumed as corrupt or invalid, so we just blow it away if // it exists. try!(config.shell().status("Packaging", pkg.package_id().to_string())); - let tmp_dst = dir.join(format!(".{}", filename)); - let _ = fs::remove_file(&tmp_dst); - try!(tar(&pkg, &src, config, &tmp_dst, &filename).chain_error(|| { + try!(dst.file().set_len(0)); + try!(tar(&pkg, &src, config, dst.file(), &filename).chain_error(|| { human("failed to prepare local package for uploading") })); if verify { - try!(run_verify(config, &pkg, &tmp_dst).chain_error(|| { + try!(dst.seek(SeekFrom::Start(0))); + try!(run_verify(config, &pkg, dst.file()).chain_error(|| { human("failed to verify package tarball") })) } - try!(fs::rename(&tmp_dst, &dst).chain_error(|| { - human("failed to move temporary tarball into final location") - })); + try!(dst.seek(SeekFrom::Start(0))); + { + let src_path = dst.path(); + let dst_path = dst.parent().join(&filename); + try!(fs::rename(&src_path, &dst_path).chain_error(|| { + human("failed to move temporary tarball into final location") + })); + } Ok(Some(dst)) } @@ -103,26 +112,17 @@ fn check_metadata(pkg: &Package, config: &Config) -> CargoResult<()> { fn tar(pkg: &Package, src: &PathSource, config: &Config, - dst: &Path, + dst: &File, filename: &str) -> CargoResult<()> { - if fs::metadata(&dst).is_ok() { - bail!("destination already exists: {}", dst.display()) - } - - try!(fs::create_dir_all(dst.parent().unwrap())); - - let tmpfile = try!(File::create(dst)); - // Prepare the encoder and its header let filename = Path::new(filename); let encoder = GzBuilder::new().filename(try!(util::path2bytes(filename))) - .write(tmpfile, Compression::Best); + .write(dst, Compression::Best); // Put all package files into a compressed archive let mut ar = Builder::new(encoder); let root = pkg.root(); for file in try!(src.list_files(pkg)).iter() { - if &**file == dst { continue } let relative = util::without_prefix(&file, &root).unwrap(); try!(check_filename(relative)); let relative = try!(relative.to_str().chain_error(|| { @@ -173,11 +173,13 @@ fn tar(pkg: &Package, Ok(()) } -fn run_verify(config: &Config, pkg: &Package, tar: &Path) +fn run_verify(config: &Config, + pkg: &Package, + tar: &File) -> CargoResult<()> { try!(config.shell().status("Verifying", pkg)); - let f = try!(GzDecoder::new(try!(File::open(tar)))); + let f = try!(GzDecoder::new(tar)); let dst = pkg.root().join(&format!("target/package/{}-{}", pkg.name(), pkg.version())); if fs::metadata(&dst).is_ok() { diff --git a/src/cargo/ops/cargo_rustc/layout.rs b/src/cargo/ops/cargo_rustc/layout.rs index 4add3a758..978090658 100644 --- a/src/cargo/ops/cargo_rustc/layout.rs +++ b/src/cargo/ops/cargo_rustc/layout.rs @@ -50,7 +50,7 @@ use std::io; use std::path::{PathBuf, Path}; use core::{Package, Target}; -use util::Config; +use util::{Config, FileLock, CargoResult, Filesystem}; use util::hex::short_hash; pub struct Layout { @@ -60,6 +60,7 @@ pub struct Layout { build: PathBuf, fingerprint: PathBuf, examples: PathBuf, + _lock: FileLock, } pub struct LayoutProxy<'a> { @@ -68,8 +69,10 @@ pub struct LayoutProxy<'a> { } impl Layout { - pub fn new(config: &Config, pkg: &Package, triple: Option<&str>, - dest: &str) -> Layout { + pub fn new(config: &Config, + pkg: &Package, + triple: Option<&str>, + dest: &str) -> CargoResult { let mut path = config.target_dir(pkg); // Flexible target specifications often point at filenames, so interpret // the target triple as a Path and then just use the file stem as the @@ -78,18 +81,25 @@ impl Layout { path.push(Path::new(triple).file_stem().unwrap()); } path.push(dest); - Layout::at(path) + Layout::at(config, path) } - pub fn at(root: PathBuf) -> Layout { - Layout { + pub fn at(config: &Config, root: Filesystem) -> CargoResult { + // For now we don't do any more finer-grained locking on the artifact + // directory, so just lock the entire thing for the duration of this + // compile. + let lock = try!(root.open_rw(".cargo-lock", config, "build directory")); + let root = root.into_path_unlocked(); + + Ok(Layout { deps: root.join("deps"), native: root.join("native"), build: root.join("build"), fingerprint: root.join(".fingerprint"), examples: root.join("examples"), root: root, - } + _lock: lock, + }) } pub fn prepare(&mut self) -> io::Result<()> { diff --git a/src/cargo/ops/cargo_rustc/mod.rs b/src/cargo/ops/cargo_rustc/mod.rs index 80bff8c3e..59e649f55 100644 --- a/src/cargo/ops/cargo_rustc/mod.rs +++ b/src/cargo/ops/cargo_rustc/mod.rs @@ -3,12 +3,12 @@ use std::env; use std::ffi::{OsStr, OsString}; use std::fs; use std::io::prelude::*; -use std::path::{self, PathBuf, Path}; +use std::path::{self, PathBuf}; use std::sync::Arc; use core::{Package, PackageId, PackageSet, Target, Resolve}; use core::{Profile, Profiles}; -use util::{self, CargoResult, human, Filesystem}; +use util::{self, CargoResult, human}; use util::{Config, internal, ChainError, profile, join_paths}; use self::job::{Job, Work}; @@ -80,17 +80,13 @@ pub fn compile_targets<'a, 'cfg: 'a>(pkg_targets: &'a PackagesToBuild<'a>, let dest = if build_config.release {"release"} else {"debug"}; let root = try!(packages.get(resolve.root())); - let host_layout = Layout::new(config, root, None, &dest); - let target_layout = build_config.requested_target.as_ref().map(|target| { - layout::Layout::new(config, root, Some(&target), &dest) - }); - - // For now we don't do any more finer-grained locking on the artifact - // directory, so just lock the entire thing for the duration of this - // compile. - let fs = Filesystem::new(host_layout.root().to_path_buf()); - let path = Path::new(".cargo-lock"); - let _lock = try!(fs.open_rw(path, config, "build directory")); + let host_layout = try!(Layout::new(config, root, None, &dest)); + let target_layout = match build_config.requested_target.as_ref() { + Some(target) => { + Some(try!(layout::Layout::new(config, root, Some(&target), &dest))) + } + None => None, + }; let mut cx = try!(Context::new(resolve, packages, config, host_layout, target_layout, diff --git a/src/cargo/ops/registry.rs b/src/cargo/ops/registry.rs index a02b80a6b..266e4accf 100644 --- a/src/cargo/ops/registry.rs +++ b/src/cargo/ops/registry.rs @@ -1,6 +1,6 @@ use std::collections::HashMap; use std::env; -use std::fs; +use std::fs::{self, File}; use std::io::prelude::*; use std::iter::repeat; use std::path::{Path, PathBuf}; @@ -51,7 +51,7 @@ pub fn publish(manifest_path: &Path, // Upload said tarball to the specified destination try!(config.shell().status("Uploading", pkg.package_id().to_string())); - try!(transmit(&pkg, &tarball, &mut registry)); + try!(transmit(&pkg, tarball.file(), &mut registry)); Ok(()) } @@ -74,7 +74,7 @@ fn verify_dependencies(pkg: &Package, registry_src: &SourceId) Ok(()) } -fn transmit(pkg: &Package, tarball: &Path, registry: &mut Registry) +fn transmit(pkg: &Package, tarball: &File, registry: &mut Registry) -> CargoResult<()> { let deps = pkg.dependencies().iter().map(|dep| { NewCrateDependency { diff --git a/src/cargo/util/config.rs b/src/cargo/util/config.rs index 66268be87..05e53c40d 100644 --- a/src/cargo/util/config.rs +++ b/src/cargo/util/config.rs @@ -30,7 +30,7 @@ pub struct Config { cwd: PathBuf, rustc: PathBuf, rustdoc: PathBuf, - target_dir: RefCell>, + target_dir: RefCell>, } impl Config { @@ -110,14 +110,14 @@ impl Config { pub fn cwd(&self) -> &Path { &self.cwd } - pub fn target_dir(&self, pkg: &Package) -> PathBuf { + pub fn target_dir(&self, pkg: &Package) -> Filesystem { self.target_dir.borrow().clone().unwrap_or_else(|| { - pkg.root().join("target") + Filesystem::new(pkg.root().join("target")) }) } - pub fn set_target_dir(&self, path: &Path) { - *self.target_dir.borrow_mut() = Some(path.to_owned()); + pub fn set_target_dir(&self, path: Filesystem) { + *self.target_dir.borrow_mut() = Some(path); } fn get(&self, key: &str) -> CargoResult> { @@ -327,9 +327,9 @@ impl Config { fn scrape_target_dir_config(&mut self) -> CargoResult<()> { if let Some(dir) = env::var_os("CARGO_TARGET_DIR") { - *self.target_dir.borrow_mut() = Some(self.cwd.join(dir)); + *self.target_dir.borrow_mut() = Some(Filesystem::new(self.cwd.join(dir))); } else if let Some(val) = try!(self.get_path("build.target-dir")) { - *self.target_dir.borrow_mut() = Some(val.val); + *self.target_dir.borrow_mut() = Some(Filesystem::new(val.val)); } Ok(()) } diff --git a/src/cargo/util/flock.rs b/src/cargo/util/flock.rs index d40978dbc..98a276723 100644 --- a/src/cargo/util/flock.rs +++ b/src/cargo/util/flock.rs @@ -1,7 +1,7 @@ use std::fs::{self, File, OpenOptions}; use std::io::*; use std::io; -use std::path::{Path, PathBuf}; +use std::path::{Path, PathBuf, Display}; use term::color::CYAN; use fs2::{FileExt, lock_contended_error}; @@ -102,7 +102,7 @@ impl Drop for FileLock { /// The `Path` of a filesystem cannot be learned unless it's done in a locked /// fashion, and otherwise functions on this structure are prepared to handle /// concurrent invocations across multiple instances of Cargo. -#[derive(Clone)] +#[derive(Clone, Debug)] pub struct Filesystem { root: PathBuf, } @@ -119,6 +119,11 @@ impl Filesystem { Filesystem::new(self.root.join(other)) } + /// Like `Path::push`, pushes a new path component onto this filesystem. + pub fn push>(&mut self, other: T) { + self.root.push(other); + } + /// Consumes this filesystem and returns the underlying `PathBuf`. /// /// Note that this is a relatively dangerous operation and should be used @@ -135,6 +140,12 @@ impl Filesystem { return create_dir_all(&self.root); } + /// Returns an adaptor that can be used to print the path of this + /// filesystem. + pub fn display(&self) -> Display { + self.root.display() + } + /// Opens exclusive access to a file, returning the locked version of a /// file. /// diff --git a/src/crates-io/lib.rs b/src/crates-io/lib.rs index fd98a772c..10ab68a72 100644 --- a/src/crates-io/lib.rs +++ b/src/crates-io/lib.rs @@ -4,10 +4,9 @@ extern crate rustc_serialize; use std::collections::HashMap; use std::fmt; -use std::fs::{self, File}; +use std::fs::File; use std::io::prelude::*; use std::io::{self, Cursor}; -use std::path::Path; use std::result; use curl::http; @@ -143,7 +142,7 @@ impl Registry { Ok(try!(json::decode::(&body)).users) } - pub fn publish(&mut self, krate: &NewCrate, tarball: &Path) -> Result<()> { + pub fn publish(&mut self, krate: &NewCrate, tarball: &File) -> Result<()> { let json = try!(json::encode(krate)); // Prepare the body. The format of the upload request is: // @@ -151,7 +150,7 @@ impl Registry { // (metadata for the package) // // - let stat = try!(fs::metadata(tarball).map_err(Error::Io)); + let stat = try!(tarball.metadata().map_err(Error::Io)); let header = { let mut w = Vec::new(); w.extend([ @@ -169,7 +168,6 @@ impl Registry { ].iter().map(|x| *x)); w }; - let tarball = try!(File::open(tarball).map_err(Error::Io)); let size = stat.len() as usize + header.len(); let mut body = Cursor::new(header).chain(tarball); diff --git a/tests/test_cargo_concurrent.rs b/tests/test_cargo_concurrent.rs index 4e8c1a8db..99dc624c0 100644 --- a/tests/test_cargo_concurrent.rs +++ b/tests/test_cargo_concurrent.rs @@ -8,7 +8,7 @@ use std::thread; use git2; use hamcrest::{assert_that, existing_file}; -use support::{execs, project, ERROR}; +use support::{execs, project, ERROR, COMPILING}; use support::git; use support::registry::Package; use test_cargo_install::{cargo_home, has_installed_exe}; @@ -350,3 +350,35 @@ test!(killing_cargo_releases_the_lock { assert!(!a.status.success()); assert_that(b, execs().with_status(0)); }); + +test!(debug_release_ok { + let p = project("foo") + .file("Cargo.toml", r#" + [package] + name = "foo" + authors = [] + version = "0.0.0" + "#) + .file("src/main.rs", "fn main() {}"); + p.build(); + + assert_that(p.cargo("build"), execs().with_status(0)); + fs::remove_dir_all(p.root().join("target")).unwrap(); + + let mut a = p.cargo("build").build_command(); + let mut b = p.cargo("build").arg("--release").build_command(); + a.stdout(Stdio::piped()).stderr(Stdio::piped()); + b.stdout(Stdio::piped()).stderr(Stdio::piped()); + let a = a.spawn().unwrap(); + let b = b.spawn().unwrap(); + let a = thread::spawn(move || a.wait_with_output().unwrap()); + let b = b.wait_with_output().unwrap(); + let a = a.join().unwrap(); + + assert_that(a, execs().with_status(0).with_stdout(&format!("\ +{compiling} foo v0.0.0 [..] +", compiling = COMPILING))); + assert_that(b, execs().with_status(0).with_stdout(&format!("\ +{compiling} foo v0.0.0 [..] +", compiling = COMPILING))); +}); -- 2.30.2